Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Flatpak: Create Autostart File Via Background Portal #681

Merged
merged 6 commits into from
Jun 13, 2023
Merged

Flatpak: Create Autostart File Via Background Portal #681

merged 6 commits into from
Jun 13, 2023

Conversation

Marukesu
Copy link
Contributor

with this calendar will call the background portal to create the daemon autostart on the first launch when sandboxed.

@danirabbit danirabbit requested a review from a team July 20, 2021 19:23
@danirabbit

This comment has been minimized.

Sign in to view
@Marukesu

This comment has been minimized.

Sign in to view
@marbetschar marbetschar mentioned this pull request Sep 6, 2021
Merged
2 tasks
@marbetschar
Copy link
Member

@Marukesu FYI I added a PR which unifies the daemon and regular app into the same binary. So we may have to rework this a little once this is merged: #702

@marbetschar marbetschar mentioned this pull request Sep 27, 2021
Closed
@danirabbit
Copy link
Member

@Marukesu can you update this/resolve conflicts since there's now a single binary?

@Marukesu Marukesu mentioned this pull request Dec 16, 2021
Closed
@Marukesu
Copy link
Contributor Author

Updated the PR, now this will show the dialog in two cases:

  • during launch with --background.
  • when closing the window.

Also, the daemon is always started now, so that we can show notifications when the application is open, even if we don't have permission to run in background.

@danirabbit
Copy link
Member

Is there a reason to only use the background portal when running in Flatpak? Can we use it all the time?

@Marukesu
Copy link
Contributor Author

Is there a reason to only use the background portal when running in Flatpak? Can we use it all the time?

The GTK portal backend uses the sandboxed app id to name the autostart file, but for host apps it is empty, so they create an unnamed .desktop that is replaced by others application on host that calls the portal (see flatpak/xdg-desktop-portal#650).

@Marukesu Marukesu mentioned this pull request Dec 18, 2021
Closed
jeremypw
jeremypw reviewed Jan 12, 2022
View reviewed changes
src/Application.vala Outdated Show resolved Hide resolved
jeremypw
jeremypw reviewed Jan 12, 2022
View reviewed changes
src/MainWindow.vala Outdated Show resolved Hide resolved
jeremypw
jeremypw previously requested changes Jan 12, 2022
View reviewed changes
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that after removing the existing Calendar installation with AppCenter and renaming the existing autostart file and installing a flatpak built from this PR and logging out and in again the following:

  1. No daemon was running initially
  2. Running the flatpak with flatpak run launched the program but did not start the daemon
  3. Closing the window triggered a dialog requesting permission to run in background
  4. After giving permission the window closed but the daemon did not start
  5. After logging out and back in, the daemon did autostart.
  6. It was not possible to launch Calendar from the wingpanel indicator but launching from the dock and Applications Menu worked. (Is this an existing bug with flatpak/wingpanel?)

I would expect the daemon to start after giving it permission to do so and not have to log out and in.

@Marukesu
Copy link
Contributor Author

  1. Running the flatpak with flatpak run launched the program but did not start the daemon

I can't reproduce this here, starting the application should start the daemon, the only diference from master here is that starting the daemon is done before the --background check.

  1. After giving permission the window closed but the daemon did not start

The last commit should fix that, for some reason, chaining up base.delete_event () was causing an segfault.

  1. It was not possible to launch Calendar from the wingpanel indicator but launching from the dock and Applications Menu worked. (Is this an existing bug with flatpak/wingpanel?)

Looking at the code in the indicator, it tries to launch io.elementary.calendar --show-day, so i don't expect it to work with the sandboxed calendar.

@Marukesu Marukesu mentioned this pull request Apr 26, 2023
Closed
@leolost2605 leolost2605 mentioned this pull request May 14, 2023
Merged
4 tasks
@danirabbit
Copy link
Member

@Marukesu wanna update this again and remove the flatpak check since the background portal can work with host apps now? :)

@Marukesu Marukesu force-pushed the background-portal branch 3 times, most recently from 81c0b81 to 14d3807 Compare May 28, 2023 18:54
@Marukesu
Copy link
Contributor Author

@danirabbit this should be good to review again.

danirabbit
danirabbit requested changes Jun 8, 2023
View reviewed changes
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it would simplify things quite a bit to use LibPortal like we did for Mail?

src/Application.vala Outdated Show resolved Hide resolved
@Marukesu Marukesu force-pushed the background-portal branch from bbaa582 to 4389670 Compare June 11, 2023 03:26
Marukesu and others added 5 commits June 11, 2023 00:35
@Marukesu Marukesu force-pushed the background-portal branch from 4389670 to 147228e Compare June 11, 2023 03:37
danirabbit
danirabbit reviewed Jun 12, 2023
View reviewed changes
src/Application.vala
@@ -61,10 +63,17 @@ namespace Maya {
}

protected override void activate () {
new Calendar.TodayEventMonitor ();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be done in activate? it looks like Mail does the equivalent in startup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, Calendar doesn't have a startup method. This work since TodayEventMonitor is marked as a [SingleInstance] class and the construct method will only be called one time.

danirabbit
danirabbit approved these changes Jun 13, 2023
View reviewed changes
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go 🚀 Thanks for working on this!

@danirabbit danirabbit merged commit a6951f8 into master Jun 13, 2023
@danirabbit danirabbit deleted the background-portal branch June 13, 2023 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danirabbit danirabbit danirabbit approved these changes

@jeremypw jeremypw jeremypw left review comments

@marbetschar marbetschar Awaiting requested review from marbetschar

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Background Apps
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Marukesu @danirabbit @marbetschar @jeremypw